Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: Support more PASSWORD REQUIRE CURRENT options | tidb-test=pr/2363 #54683

Open
wants to merge 41 commits into
base: master
Choose a base branch
from

Conversation

dveeden
Copy link
Contributor

@dveeden dveeden commented Jul 17, 2024

What problem does this PR solve?

Issue Number: close #54660

Problem Summary:

Support a global (password_require_current) and per account (mysql.user.Password_require_current) policy for requiring the current password when changing the password.

What changed and how does it work?

  1. To create/modify the per user policy use:
CREATE USER ... PASSWORD REQUIRE CURRENT;
CREATE USER ... PASSWORD REQUIRE CURRENT OPTIONAL;
CREATE USER ... PASSWORD REQUIRE CURRENT DEFAULT;
ALTER USER ... PASSWORD REQUIRE CURRENT;
ALTER USER ... PASSWORD REQUIRE CURRENT OPTIONAL;
ALTER USER ... PASSWORD REQUIRE CURRENT DEFAULT;
  1. To change the system policy:
SET GLOBAL password_require_current=OFF;
SET GLOBAL password_require_current=ON;
  1. To specify a password when changing the password:
ALTER USER ... IDENTIFIED BY 'newpwd' REPLACE 'curentpwd';

parser:

  • Support PASSWORD REQUIRE CURRENT
  • Support PASSWORD REQUIRE CURRENT OPTIONAL
  • Support ALTER USER...IDENTIFIED BY ... REPLACE ...
  • Support ALTER USER...IDENTIFIED WITH ... BY ... REPLACE ...
  • Support ALTER USER() IDENTIFIED BY ... REPLACE ...
  • Add ErrIncorrectCurrentPassword
  • Add ErrMissingCurrentPassword
  • Add ErrCurrentPasswordNotRequired

session:

  • Add Password_require_current column to mysql.user

sessionctx/variable:

  • Add password_require_current system variable

executor:

  • Include "PASSWORD REQUIRE CURRENT ..." in SHOW CREATE USER output
  • Support modifying accounts with ALTER USER ... PASSWORD REQUIRE CURRENT ...
  • Check current password in ALTER USER

errno:

  • Add ErrIncorrectCurrentPassword
  • Add ErrMissingCurrentPassword
  • Add ErrCurrentPasswordNotRequired

privilege:

  • Load current password policy from mysql.user table
  • Check current password

Questions and remarks

  • TestAbnormalMySQLTable has a comment that says it is testing with a mysql.user table synchronized from MySQL. However the supplied data looks like a TiDB mysql.user table instead. Is this test correct? Should this be multiple tests, with MySQL 5.7,8.0 and 8.4? Maybe 9.0? @CbcWestwolf ? TestAbnormalMySQLTable has multiple issues. #54924
  • Looks like there is no interface or good abstraction for authentication plugins. Now (*UserPrivilege).checkPassword() for example needs to do something different based on what authentication plugin is being used. Maybe we can make auth.CheckHashingPassword() more generic, but then we should probably change the name. Maybe an interface is best, but probably that's out of scope for this PR.
  • show.go has a "FIXME: the returned string is not escaped safely" comment. Maybe we should fix this outside of this PR?
  • This should work for mysql_native_password, tidb_sm3_password and caching_sha2_password. For other methods more testing and work might be needed.
  • Looks like we still have the PASSWORD() function and SET PASSWORD = PASSWORD(...) from the MySQL 5.7 era. Might be good to start the process of removing this (outside of this PR).

Documentation needed

  • ALTER USER docs to include:
    • Add: ALTER USER...PASSWORD REQUIRE CURRENT ...
    • Add: ALTER USER...IDENTIFIED BY... REPLACE ...
  • System variable docs
    • Add: password_require_current
  • System table docs
    • Update mysql.user docs

TODO

  • Check impact on audit logging for new field that contains a password
  • Check impact on log redact for a new field that contains a password
  • Check impact on BR for new column in mysql.user
  • Check impact on TiCDC for new column in mysql.user
  • Check impact on DM for new column in mysql.user

For log redaction and audit logging:

These take care of this for SET PASSWORD and ALTER USER:

pkg/parser/ast/misc.go:func (n *SetPwdStmt) SecureText() string {
pkg/parser/ast/misc.go:func (n *AlterUserStmt) SecureText() string {
  • Both don't really need an update but (*AlterUserStmt).SecureText() was updated anyway to make the intention clear.
  • The REPLACE <password> syntax is only used with SET PASSWORD and ALTER USER statements, which both already deal with passwords.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Support for a password policies that require the current password when changing passwords has been added.

Copy link

ti-chi-bot bot commented Jul 17, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ti-chi-bot ti-chi-bot bot added do-not-merge/invalid-title release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-tests-checked do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 17, 2024
Copy link

tiprow bot commented Jul 17, 2024

Hi @dveeden. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@dveeden dveeden changed the title Support more PASSWORD REQUIRE CURRENT options *: Support more PASSWORD REQUIRE CURRENT options Jul 17, 2024
@dveeden
Copy link
Contributor Author

dveeden commented Jul 17, 2024

/test all

Copy link

tiprow bot commented Jul 17, 2024

@dveeden: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test all

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@dveeden
Copy link
Contributor Author

dveeden commented Jul 17, 2024

/test all

Copy link

tiprow bot commented Jul 17, 2024

@dveeden: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test all

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@dveeden
Copy link
Contributor Author

dveeden commented Jul 17, 2024

/ok-to-test

@ti-chi-bot ti-chi-bot bot added ok-to-test Indicates a PR is ready to be tested. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jul 17, 2024
Copy link

codecov bot commented Jul 17, 2024

Codecov Report

Attention: Patch coverage is 58.92857% with 69 lines in your changes missing coverage. Please review.

Project coverage is 57.4296%. Comparing base (50d73f8) to head (2796d37).
Report is 140 commits behind head on master.

Additional details and impacted files
@@                Coverage Diff                @@
##             master     #54683         +/-   ##
=================================================
- Coverage   73.2879%   57.4296%   -15.8583%     
=================================================
  Files          1638       1836        +198     
  Lines        453742     690401     +236659     
=================================================
+ Hits         332538     396495      +63957     
- Misses       100823     266820     +165997     
- Partials      20381      27086       +6705     
Flag Coverage Δ
integration 24.1840% <4.3750%> (?)
unit 72.5098% <49.3750%> (+0.0069%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 52.9478% <ø> (ø)
parser ∅ <ø> (∅)
br 62.0882% <ø> (+16.0861%) ⬆️

@dveeden
Copy link
Contributor Author

dveeden commented Jul 17, 2024

/test all

@dveeden
Copy link
Contributor Author

dveeden commented Jul 17, 2024

/test all

@dveeden dveeden changed the title *: Support more PASSWORD REQUIRE CURRENT options *: Support more PASSWORD REQUIRE CURRENT options | tidb-test=pr/2363 Jul 17, 2024
@dveeden
Copy link
Contributor Author

dveeden commented Jul 17, 2024

/retest

@dveeden
Copy link
Contributor Author

dveeden commented Jul 17, 2024

/test all

@dveeden
Copy link
Contributor Author

dveeden commented Jul 25, 2024

/retest

@dveeden
Copy link
Contributor Author

dveeden commented Jul 26, 2024

Maybe more tests should be ported to cover the scenarios in password-reverification-policy and t/password_require_current.test

That file relies heavily on --source, which mysql-tester doesn't support yet. I've created pingcap/mysql-tester#128 to try to address that.

@ti-chi-bot ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2024
@ti-chi-bot ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2024
@dveeden
Copy link
Contributor Author

dveeden commented Jul 26, 2024

/retest

@dveeden
Copy link
Contributor Author

dveeden commented Jul 29, 2024

/retest

@dveeden
Copy link
Contributor Author

dveeden commented Jul 29, 2024

/retest

1 similar comment
@dveeden
Copy link
Contributor Author

dveeden commented Jul 29, 2024

/retest

@dveeden dveeden requested a review from BornChanger July 30, 2024 06:23
Create_Tablespace_Priv ENUM('N','Y') NOT NULL DEFAULT 'N',
Password_reuse_history smallint unsigned DEFAULT NULL,
Password_reuse_time smallint unsigned DEFAULT NULL,
Password_require_current ENUM('N','Y') DEFAULT NULL,
Copy link
Contributor

@BornChanger BornChanger Jul 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about put Password_require_current under column of user_attirbutes for better cross-version system table compatibility?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to put Password_require_current as a new field in mysql.user for better compatibility with mysql's
https://dev.mysql.com/doc/mysql-security-excerpt/8.0/en/grant-tables.html#grant-tables-user-db

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let's do this in the same way as MySQL unless there is a very strong reason not to do that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems pr #39460 has addressed the compatibility problem here. so, i am fine with the change. but please make sure to do a cross version backup restore testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A backup of TiDB-with-this-PR to a vanilla TiDB returns this:

dvaneeden@dve-carbon:~$ tiup br restore full -s /tmp/b_full/
Checking updates for component br... Starting component br: /home/dvaneeden/.tiup/components/br/v8.3.0/br restore full -s /tmp/b_full/
Detail BR log in /tmp/br.log.2024-08-22T09.43.39+0200 
[2024/08/22 09:43:41.949 +02:00] [INFO] [collector.go:77] ["Full Restore failed summary"] [total-ranges=0] [ranges-succeed=0] [ranges-failed=0]
#######################################################################
# the target cluster is not compatible with the backup data,
# you can use '--with-sys-table=false' to skip restoring system tables
#######################################################################
Error: missing column in cluster data, table: user, col: Password_require_current enum('N','Y'): [BR:Restore:ErrRestoreIncompatibleSys]incompatible system table

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can make BR to succeed with this patch:

diff --git a/br/pkg/restore/snap_client/systable_restore.go b/br/pkg/restore/snap_client/systable_restore.go
index a99d9925b1..7b758422d2 100644
--- a/br/pkg/restore/snap_client/systable_restore.go
+++ b/br/pkg/restore/snap_client/systable_restore.go
@@ -276,7 +276,7 @@ func (rc *SnapClient) replaceTemporaryTableToSystable(ctx context.Context, ti *m
                        zap.Stringer("schema", db.Name))
                // target column order may different with source cluster
                columnNames := make([]string, 0, len(ti.Columns))
-               for _, col := range ti.Columns {
+               for _, col := range db.ExistingTables[tableName].Columns {
                        columnNames = append(columnNames, utils.EncloseName(col.Name.L))
                }
                colListStr := strings.Join(columnNames, ",")
@@ -384,13 +384,19 @@ func CheckSysTableCompatibility(dom *domain.Domain, tables []*metautil.Table) er
                                col := backupTi.Columns[i]
                                clusterCol := clusterColMap[col.Name.L]
                                if clusterCol == nil {
-                                       log.Error("missing column in cluster data",
-                                               zap.Stringer("table", table.Info.Name),
-                                               zap.String("col", fmt.Sprintf("%s %s", col.Name, col.FieldType.String())))
-                                       return errors.Annotatef(berrors.ErrRestoreIncompatibleSys,
-                                               "missing column in cluster data, table: %s, col: %s %s",
-                                               table.Info.Name.O,
-                                               col.Name, col.FieldType.String())
+                                       if col.Name.L == "password_require_current" {
+                                               log.Error("ignoring missing column in cluster data",
+                                                       zap.Stringer("table", table.Info.Name),
+                                                       zap.String("col", fmt.Sprintf("%s %s", col.Name, col.FieldType.String())))
+                                       } else {
+                                               log.Error("missing column in cluster data",
+                                                       zap.Stringer("table", table.Info.Name),
+                                                       zap.String("col", fmt.Sprintf("%s %s", col.Name, col.FieldType.String())))
+                                               return errors.Annotatef(berrors.ErrRestoreIncompatibleSys,
+                                                       "missing column in cluster data, table: %s, col: %s %s",
+                                                       table.Info.Name.O,
+                                                       col.Name, col.FieldType.String())
+                                       }
                                }
                        }
                }

This patch:

  • Makes the "missing column in cluster data" non-fatal for the "password_require_current" column
  • Uses the list of columns for the target table instead of the list of columns of the source table. Maybe this should check for columns that are present in both if we actually want to do it like this.

However the result is that the PASSWORD REQUIRE CURRENT (DEFAULT|OPTIONAL|) part of the user definition gets lost as there isn't a column for that. I think this might be acceptable but not supporting this and requiring --with-sys-table=false might be a good option as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restoring a backup of TiDB v8.3.0 vanilla on TiDB-with-this-PR works:

From the logs:

[2024/08/22 10:49:10.696 +02:00] [WARN] [systable_restore.go:349] ["missing column in backup data"] [table=user] [col="Password_require_current enum('N','Y')"]

@ti-chi-bot ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2024
Copy link

ti-chi-bot bot commented Aug 10, 2024

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@dveeden
Copy link
Contributor Author

dveeden commented Aug 21, 2024

/retest

@dveeden
Copy link
Contributor Author

dveeden commented Aug 21, 2024

/retest

@dveeden dveeden removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2024
Copy link

ti-chi-bot bot commented Sep 6, 2024

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@ti-chi-bot ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 6, 2024
Copy link

ti-chi-bot bot commented Oct 29, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign benmeadowcroft, bornchanger for approval, ensuring that each of them provides their approval before proceeding. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

ti-chi-bot bot commented Nov 18, 2024

@dveeden: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
idc-jenkins-ci-tidb/check_dev 2796d37 link true /test check-dev
idc-jenkins-ci-tidb/check_dev_2 2796d37 link true /test check-dev2
pull-integration-e2e-test 2796d37 link true /test pull-integration-e2e-test

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a PR is ready to be tested. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Full support for "PASSWORD REQUIRE CURRENT"
4 participants